Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Join assets from verticals in the final join point #43627

Merged
merged 23 commits into from
Oct 17, 2024

Conversation

MilenaHristova
Copy link
Member

Issue: dotnet/source-build#4199

  • Adds JoinVerticals task that merges the packages and blobs from all verticals and creates a MergedManifest.xml
  • Adds a new stage in the unified build pipeline that runs the JoinVerticals task after all build passes are completed

Test run: https://dev.azure.com/dnceng/internal/_build/results?buildId=2544654&view=results

Files from verticals are downloaded by individual requests to AzDO api; files from the first vertical are copied directly

@directhex
Copy link
Contributor

We definitely need to filter the verticals list before we merge, since we have some VMR legs we don't intend to publish. We also seem to have some cases where the wrong RID is being used, which is a bit scary, and we're going for a first-come-first-served approach on downloads, so:

  Taking Package 'Microsoft.AspNetCore.App.Runtime.linux-x64.9.0.0-rc.1.24414.4.nupkg' from 'AzureLinux_x64_Cross_Alpine_Mono_x64'

That's scary. a) the RID is wrong, it should be linux-musl-x64 (not your fault, that's a VMR bug) b) actually we don't want to use any of the _Mono_ verticals for final join, as far as I'm aware (and since this package name was pulled from this vertical, the correct package wasn't pulled from AzureLinux_x64_Cross_x64)

@directhex
Copy link
Contributor

We definitely need to filter the verticals list before we merge, since we have some VMR legs we don't intend to publish. We also seem to have some cases where the wrong RID is being used, which is a bit scary, and we're going for a first-come-first-served approach on downloads, so:

  Taking Package 'Microsoft.AspNetCore.App.Runtime.linux-x64.9.0.0-rc.1.24414.4.nupkg' from 'AzureLinux_x64_Cross_Alpine_Mono_x64'

That's scary. a) the RID is wrong, it should be linux-musl-x64 (not your fault, that's a VMR bug) b) actually we don't want to use any of the _Mono_ verticals for final join, as far as I'm aware (and since this package name was pulled from this vertical, the correct package wasn't pulled from AzureLinux_x64_Cross_x64)

I filed dotnet/source-build#4628 for the issue in (a)

@MilenaHristova
Copy link
Member Author

We definitely need to filter the verticals list before we merge, since we have some VMR legs we don't intend to publish. We also seem to have some cases where the wrong RID is being used, which is a bit scary, and we're going for a first-come-first-served approach on downloads, so:

  Taking Package 'Microsoft.AspNetCore.App.Runtime.linux-x64.9.0.0-rc.1.24414.4.nupkg' from 'AzureLinux_x64_Cross_Alpine_Mono_x64'

That's scary. a) the RID is wrong, it should be linux-musl-x64 (not your fault, that's a VMR bug) b) actually we don't want to use any of the _Mono_ verticals for final join, as far as I'm aware (and since this package name was pulled from this vertical, the correct package wasn't pulled from AzureLinux_x64_Cross_x64)

would it make sense to list the verticals to join implicitly as dependencies of the final join job? We were thinking about that option but I was under the impression that all verticals need to be joined and that maintaining the list would be difficult

@mmitche
Copy link
Member

mmitche commented Sep 24, 2024

We definitely need to filter the verticals list before we merge, since we have some VMR legs we don't intend to publish. We also seem to have some cases where the wrong RID is being used, which is a bit scary, and we're going for a first-come-first-served approach on downloads, so:

  Taking Package 'Microsoft.AspNetCore.App.Runtime.linux-x64.9.0.0-rc.1.24414.4.nupkg' from 'AzureLinux_x64_Cross_Alpine_Mono_x64'

That's scary. a) the RID is wrong, it should be linux-musl-x64 (not your fault, that's a VMR bug) b) actually we don't want to use any of the _Mono_ verticals for final join, as far as I'm aware (and since this package name was pulled from this vertical, the correct package wasn't pulled from AzureLinux_x64_Cross_x64)

would it make sense to list the verticals to join implicitly as dependencies of the final join job? We were thinking about that option but I was under the impression that all verticals need to be joined and that maintaining the list would be difficult

Perhaps an exclusion works better. We don't want certain verticals, but we want all by default?

@MilenaHristova
Copy link
Member Author

MilenaHristova commented Sep 25, 2024

@mmitche are we going to use JoinVerticals task only for the final build pass? I think the downloads for 2nd build pass are implemented with #43288

@mmitche
Copy link
Member

mmitche commented Sep 25, 2024

@mmitche are we going to use JoinVerticals task only for the final build pass? I think the downloads for 2nd build pass are implemented with #43288

We may need to use it for both. the join 2nd pass joins will probably have some overlapping artifacts.

/cc @directhex

@directhex
Copy link
Contributor

@mmitche are we going to use JoinVerticals task only for the final build pass? I think the downloads for 2nd build pass are implemented with #43288

We may need to use it for both. the join 2nd pass joins will probably have some overlapping artifacts.

/cc @directhex

The main thing #43288 did was allow a given job to depend on multiple other jobs' nupkg output. For example, workload creation requires all component nupkgs to exist on disk, so we need the results of the existing Windows_x64 vertical, but also the new vertical dotnet/source-build#3891 would add. The implementation is done in AzDO tasks, sequentially downloading every nupkg from the named jobs into the join point job's main packages folder.

It's not entirely obvious to me how that behaviour would be expected to intersect with this one (e.g. if a vertical creates runtime packs and uploads them, a second vertical creates aot compilers and uploads them, and a third vertical downloads the previous two uploads, adds in more packages, then uploads the full set of both the new packages and the ingested packages from earlier verticals, then the expected behaviour of the final join point isn't clear to me)

@mmitche
Copy link
Member

mmitche commented Sep 25, 2024

Some subset of the functionality is necessary. If you need Windows x86 and Window x64 as inputs to product a given vertical in pass 2, there is a very good chance that there will be some overlap of artifacts between the input verticals. We need a selection algorithm for that. The output vertical manifest is not the interesting output. The selection of inputs is.

@MilenaHristova
Copy link
Member Author

@mmitche I'm working on checking for duplicate assets in non-primary verticals and it turns out there a lot of them.

I ran the task locally on artifacts from https://dev.azure.com/dnceng/internal/_build/results?buildId=2544654&view=results and there is a lot of overlap between Ubuntu2404_x64 and AzureLinux_x64_Cross_x64 also between all the *AzureLinux* verticals

So far I'm excluding everything containing "Mono", "Pgo", "Shortstack" and "DevVersions", let me know if that's correct

@mmitche
Copy link
Member

mmitche commented Sep 26, 2024

@mmitche I'm working on checking for duplicate assets in non-primary verticals and it turns out there a lot of them.

I ran the task locally on artifacts from https://dev.azure.com/dnceng/internal/_build/results?buildId=2544654&view=results and there is a lot of overlap between Ubuntu2404_x64 and AzureLinux_x64_Cross_x64 also between all the *AzureLinux* verticals

So far I'm excluding everything containing "Mono", "Pgo", "Shortstack" and "DevVersions", let me know if that's correct

Okay super interesting. I'll take a look at the list in a few. I think for now let's print this list as info, pick the first, and open a work item to drive it down.

I'm not sure how valid it will be that we run the join job in the normal PR job. Or at least, we may want to exclude some legs entirely from the join since some portion of the legs are for validation purposes and may have expected overlap. They may not represent the official build. For instance, the DevVersions and normal ubuntu legs would be expected to produce the same assets, just at different versions.

@MilenaHristova
Copy link
Member Author

@mmitche I'm working on checking for duplicate assets in non-primary verticals and it turns out there a lot of them.
I ran the task locally on artifacts from https://dev.azure.com/dnceng/internal/_build/results?buildId=2544654&view=results and there is a lot of overlap between Ubuntu2404_x64 and AzureLinux_x64_Cross_x64 also between all the *AzureLinux* verticals
So far I'm excluding everything containing "Mono", "Pgo", "Shortstack" and "DevVersions", let me know if that's correct

Okay super interesting. I'll take a look at the list in a few. I think for now let's print this list as info, pick the first, and open a work item to drive it down.

I'm not sure how valid it will be that we run the join job in the normal PR job. Or at least, we may want to exclude some legs entirely from the join since some portion of the legs are for validation purposes and may have expected overlap. They may not represent the official build. For instance, the DevVersions and normal ubuntu legs would be expected to produce the same assets, just at different versions.

This test run prints a list https://dev.azure.com/dnceng/internal/_build/results?buildId=2547353&view=logs&j=012d8f49-bbe9-5cff-0c2d-eb37469742f3&t=a088a11a-052c-5a48-307b-2470eb67bf36&l=3378

}
else
{
fileItem = matchingFilePaths
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not redundant, can you add a comment as to its purpose?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment, this is for the sdk zip which exists in 2 places in the artifact - for example Windows_x64_Artifacts/assets/Release/dotnet-sdk-*-win-x64.zip and Windows_x64_Artifacts/assets/Release/Sdk/*/dotnet-sdk-*-win-x64.zip but it's listed only once in the vertical manifest
I assume it's getting copied somewhere after the manifest creation but couldn't find where
Comparing the full path solves the issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, and my guess is that it's not enough to initially compare with the full path in all cases?

We need to add some tests for this merging code to ensure that the desired selection is being made: dotnet/source-build#4661

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some places where Contains doesn't work - both Microsoft.DotNet.Wpf.GitHub.9.0.0-rc.1.24413.1.nupkg and runtime.win-x86.Microsoft.DotNet.Wpf.GitHub.9.0.0-rc.1.24413.1.nupkg contain the same package name
So we'd need to check both that the filename fully matches and compare the path, since we need to check the path only in few cases it made sense to do it this way

@ViktorHofer
Copy link
Member

I'd like to get #44153 merged in first and then react to that in this PR (should just be 1-2 conflicts). Mainly because testing the join point is very difficult and time consuming. Hope that's OK with everyone.

@ViktorHofer ViktorHofer enabled auto-merge (squash) October 17, 2024 18:09
@ViktorHofer ViktorHofer merged commit ea38eba into dotnet:main Oct 17, 2024
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants